Lockfile invalidation -- add metadata to generated lockfiles #12427

chrisjrn · 2021-07-26T21:00:51Z

This WIP adds the lockfile invalidation header, but does not yet consume it.

Partially addresses #12415.

chrisjrn · 2021-07-26T21:01:15Z

@Eric-Arellano Happy to take pointers here, otherwise, fyi!

chrisjrn · 2021-07-26T21:03:52Z

(It seems to work?)

chrisjrn · 2021-07-26T21:05:40Z

Notably, this implementation should make it easy to add the usage instructions automagically, by amending this line: https://github.com/pantsbuild/pants/pull/12427/files#diff-e09a7e81d00d6394fbb4336f41dde1ba1aea4b0b1af07d173124ecb65b9e9308R155

Eric-Arellano

Yay!

Eric-Arellano · 2021-07-26T23:50:19Z

src/python/pants/backend/experimental/python/lockfile.py

+        m = hashlib.sha256()
+        for requirement in self.requirements:
+            m.update(requirement.encode("utf-8"))
+        for constraint in self.interpreter_constraints:
+            m.update(str(constraint).encode("utf-8"))
+        return m.hexdigest()


Thinking out loud here..my first thought was "This seems inefficient to iterate over every element! You could hash the whole collection to avoid that".

But I suspect this is intentional to iterate over every element, and that it makes the algorithm more robust to hash collisions? Is that right? If so, maybe add a comment explaining this nuance.

Secure hashes from hashlib work on bytes objects -- you can't hash an iterable, but you can hash a bytes representation of every item of an iterable. I don't think there's any nuance here?

Hm, @stuhood thoughts?

Returning to this, under the hood, m.update(a + b) is equivalent to m.update(a); m.update(b) (see https://docs.python.org/3/library/hashlib.html) so I presume that hashlib is doing something with stringio or similar under the hood to build up the input to the digest function, which is about as efficient as what we'd end up doing.

To clarify, I'm wondering if we could do m.update(repr(self.requiremenets).encode("utf-8")), so that would not quite be the same thing. I'm pretty sure that's more likely to have a hash collision, only not sure it matters.

Using the repr of the collection would be equivalent to adding a bit of salt to the hashes; I don't think either approach meaningfully changes the probability of a collision. The chance of a collision over a 256-bit cryptographic hash is vanishingly small either way, so the presence/otherwise of a few delimiters isn't going to meaningfully change that.

One thing to consider in this is sort order. Are the requirements/constraints sorted when going into this, otherwise a change of order would yield different hashes, which I suppose is undesirable, as they would still resolve to the same thing once used, iiuc.

src/python/pants/backend/experimental/python/lockfile.py

Eric-Arellano

Thanks for adding tests!

src/python/pants/backend/experimental/python/lockfile.py

src/python/pants/backend/experimental/python/lockfile_test.py

src/python/pants/backend/experimental/python/lockfile.py

src/python/pants/backend/experimental/python/lockfile_test.py

src/python/pants/backend/experimental/python/lockfile.py

Eric-Arellano · 2021-07-27T17:40:21Z

src/python/pants/backend/experimental/python/lockfile.py

+        m = hashlib.sha256()
+        for requirement in self.requirements:
+            m.update(requirement.encode("utf-8"))
+        for constraint in self.interpreter_constraints:
+            m.update(str(constraint).encode("utf-8"))
+        return m.hexdigest()


Hm, @stuhood thoughts?

src/python/pants/backend/experimental/python/lockfile_metadata.py

Eric-Arellano

Looks great! Thanks.

The lockfiles should maybe be regenerated? But it's a tedious process..you have to run on a machine w/ Py36-Py39 (not M1) and manually restore the headers for how to regen. I'm personally fine with punting on it for now and one of us implementing #12382 first.

Eric-Arellano · 2021-07-28T19:45:11Z

src/python/pants/backend/experimental/python/lockfile.py

+        m = hashlib.sha256()
+        for requirement in self.requirements:
+            m.update(requirement.encode("utf-8"))
+        for constraint in self.interpreter_constraints:
+            m.update(str(constraint).encode("utf-8"))
+        return m.hexdigest()


To clarify, I'm wondering if we could do m.update(repr(self.requiremenets).encode("utf-8")), so that would not quite be the same thing. I'm pretty sure that's more likely to have a hash collision, only not sure it matters.

src/python/pants/backend/experimental/python/lockfile_test.py

src/python/pants/backend/experimental/python/lockfile_metadata.py

chrisjrn · 2021-07-28T19:59:59Z

@Eric-Arellano Once the lockfile digests are being consumed, we can potentially add the regen information to the lockfile_metadata_header function and related tests; I don't feel like we need to regenerate the lockfiles right away

Eric-Arellano

Only small nits. Sounds like you're headed out for today, so I'll plan to merge this and then they can be fixed in your upcoming followup PR.

Thanks!

Eric-Arellano · 2021-07-29T23:19:47Z

src/python/pants/backend/experimental/python/lockfile_metadata.py

+    """Returns an invalidation digest for the given requirements and interpreter constraints."""
+    m = hashlib.sha256()
+    pres = {
+        "requirements": [str(i) for i in requirements],


They're already strings

Suggested change

"requirements": [str(i) for i in requirements],

"requirements": list(requirements),

Eric-Arellano · 2021-07-29T23:22:24Z

src/python/pants/backend/experimental/python/lockfile_metadata_test.py

+    print(
+        invalidation_digest(
+            FrozenOrderedSet(requirements), InterpreterConstraints(interpreter_constraints)
+        )
+    )


Eric-Arellano · 2021-07-29T23:22:37Z

src/python/pants/backend/experimental/python/lockfile_metadata_test.py

+    )
+
+
+def test_hash_depends_on_requirement_source():


Suggested change

def test_hash_depends_on_requirement_source():

def test_hash_depends_on_requirement_source() -> None:

chrisjrn · 2021-07-30T15:25:48Z

@Eric-Arellano I've fixed those

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

[ci skip-rust] [ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

… be simpler, I promise. # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

[ci skip-rust] [ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Closes #12415 This adds tools for consuming the lockfile invalidation digests that were added in #12427, both for lockfiles specified for the project, and for individual build tools. Behavior on invalid lockfiles can be configured to either be a warning or an error.

chrisjrn marked this pull request as draft July 26, 2021 21:00

chrisjrn requested a review from Eric-Arellano July 26, 2021 21:01

Eric-Arellano reviewed Jul 27, 2021

View reviewed changes

chrisjrn force-pushed the chrisjrn/12415-lockfile-invalidation branch from de8da6e to 00b1d91 Compare July 27, 2021 19:21

Eric-Arellano mentioned this pull request Jul 28, 2021

Make the header for our generated files useful, e.g. saying how to regen it. #12382

Closed

chrisjrn marked this pull request as ready for review July 28, 2021 19:28

Eric-Arellano reviewed Jul 28, 2021

View reviewed changes

src/python/pants/backend/experimental/python/lockfile_metadata.py Outdated Show resolved Hide resolved

Eric-Arellano approved these changes Jul 28, 2021

View reviewed changes

Eric-Arellano requested a review from stuhood July 28, 2021 19:48

chrisjrn changed the title ~~WIP: Lockfile invalidation~~ Lockfile invalidation Jul 28, 2021

chrisjrn changed the title ~~Lockfile invalidation~~ Lockfile invalidation -- add metadata to generated lockfiles Jul 28, 2021

chrisjrn mentioned this pull request Jul 29, 2021

Lockfile invalidation consumption #12448

Merged

Eric-Arellano approved these changes Jul 29, 2021

View reviewed changes

Christopher Neugebauer added 11 commits July 30, 2021 08:44

Adds hex_digest property to LockFileRequests

af5fa30

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Adds lockfile_metadata functions

30c1e51

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Adds the invalidation digest header to lockfiles

b1b6568

[ci skip-rust] [ci skip-build-wheels]

Change the intermediate filename for nicer-looking lockfiles

10632a1

[ci skip-rust] [ci skip-build-wheels]

Factors out "begin lockfile metadata" into constants

71f776f

[ci skip-rust] [ci skip-build-wheels]

Adds unit tests for lockfile invalidation helpers

47af3f1

[ci skip-rust] [ci skip-build-wheels]

Adds comment to explain lambda

fca64b5

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Code review concerns

148c29e

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Factors out metadata functions into lockfile_metadata.py. This will…

8a599a1

… be simpler, I promise. # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Improves unit tests per code review comments

7066ab2

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Fixes formatting issues

4a489bb

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Christopher Neugebauer added 5 commits July 30, 2021 08:44

Removes cicrular dependency in lockfile_metadata

08ee67b

[ci skip-rust] [ci skip-build-wheels]

Remove walrus

324dd0e

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Renamed lockfile_test.py

ba02a9a

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Updates invalidation digest output

1d517fc

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Resolve small nitpicks

3694ae3

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

chrisjrn force-pushed the chrisjrn/12415-lockfile-invalidation branch from 2ab468d to 3694ae3 Compare July 30, 2021 15:44

Eric-Arellano approved these changes Jul 30, 2021

View reviewed changes

Eric-Arellano merged commit 6ee8f27 into pantsbuild:main Jul 30, 2021

Eric-Arellano mentioned this pull request Jul 31, 2021

Prepare 2.7.0.dev2 #12475

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lockfile invalidation -- add metadata to generated lockfiles #12427

Lockfile invalidation -- add metadata to generated lockfiles #12427

chrisjrn commented Jul 26, 2021

chrisjrn commented Jul 26, 2021

chrisjrn commented Jul 26, 2021 •

edited

Loading

chrisjrn commented Jul 26, 2021

Eric-Arellano left a comment

Eric-Arellano Jul 26, 2021

chrisjrn Jul 27, 2021

Eric-Arellano Jul 27, 2021

chrisjrn Jul 28, 2021

Eric-Arellano Jul 28, 2021

chrisjrn Jul 28, 2021

kaos Jul 30, 2021

Eric-Arellano left a comment

Eric-Arellano Jul 27, 2021

Eric-Arellano left a comment

Eric-Arellano Jul 28, 2021

chrisjrn commented Jul 28, 2021

Eric-Arellano left a comment

Eric-Arellano Jul 29, 2021

Eric-Arellano Jul 29, 2021

Eric-Arellano Jul 29, 2021

chrisjrn commented Jul 30, 2021

	"requirements": [str(i) for i in requirements],
	"requirements": list(requirements),

	def test_hash_depends_on_requirement_source():
	def test_hash_depends_on_requirement_source() -> None:

Lockfile invalidation -- add metadata to generated lockfiles #12427

Lockfile invalidation -- add metadata to generated lockfiles #12427

Conversation

chrisjrn commented Jul 26, 2021

chrisjrn commented Jul 26, 2021

chrisjrn commented Jul 26, 2021 • edited Loading

chrisjrn commented Jul 26, 2021

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisjrn commented Jul 28, 2021

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisjrn commented Jul 30, 2021

chrisjrn commented Jul 26, 2021 •

edited

Loading